-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Transform] add new exclude_generated flag to GET transform #63093
[Transform] add new exclude_generated flag to GET transform #63093
Conversation
Pinging @elastic/ml-core (:ml/Transform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
public static final String FOR_EXPORT = "for_export"; | ||
public static final String ALLOW_NO_MATCH = "allow_no_match"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final String FOR_EXPORT = "for_export"; | |
public static final String ALLOW_NO_MATCH = "allow_no_match"; | |
public static final String ALLOW_NO_MATCH = "allow_no_match"; | |
public static final String FOR_EXPORT = "for_export"; |
Code look good, added 2 suggestions. I wonder about the end2end usage. The export removes the id, I think this limits usability, you might want to store transform exported configs in VCS. Without the name this isn't practical. The parser actually allows setting the id, however it must be equal to the one in the path. That means technically we could keep the id. Therefore the bigger story seems incomplete to me, we need to define how import is going to work. If we e.g. allow However, single import isn't useful either, you likely want a bulk import that takes the exact output of |
...in/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java
Outdated
Show resolved
Hide resolved
...in/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java
Show resolved
Hide resolved
Technically we could, but this defeats the idea of clone. Exporting and then easily importing in the same cluster seems like a vital use case here. The configuration object itself shouldn't reference an ID. If the users want to store the config in a VCS, the file name itself should have the ID, not the enclosed object. If they are using some VCS system, a user is either copying and setting a file name anyways, or using some automation to do this. Additionally, we already have a cluster backup mechanism. Snapshots should be used for cluster or index backup and restore.
We already have an import, it's
I would think that when experimenting with configurations in staging and then moving them to production, this is done one at a time.
I think having an We could easily update |
...in/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, 1 minor comment
if (params.paramAsBoolean(TransformField.EXCLUDE_GENERATED, false) == false) { | ||
builder.field(QUERY.getPreferredName(), queryConfig); | ||
} else if(queryConfig.equals(QueryConfig.matchAll()) == false) { | ||
builder.field(QUERY.getPreferredName(), queryConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could be done with an or and 1 if
…63093) This adds a new flag `exclude_generated` for GET transform API. This flag is useful for when a transform needs to be cloned within a cluster or exported/imported between clusters. It removes certain fields that are not able to be set via the PUT api (e.g. version, create_time). relates elastic#63055
…63947) This adds a new flag `exclude_generated` for GET transform API. This flag is useful for when a transform needs to be cloned within a cluster or exported/imported between clusters. It removes certain fields that are not able to be set via the PUT api (e.g. version, create_time). relates #63055
…63093) This adds a new flag `exclude_generated` for GET transform API. This flag is useful for when a transform needs to be cloned within a cluster or exported/imported between clusters. It removes certain fields that are not able to be set via the PUT api (e.g. version, create_time). relates elastic#63055
This adds a new flag
exclude_generated
for GET transform API.This flag is useful for when a transform needs to be cloned within a cluster or exported/imported between clusters.
It removes certain fields that are not able to be set via the PUT api (e.g. version, create_time).
relates #63055